Skip to content

Fix two places where we write into vectors with ghosts. #6841

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
bangerth wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix two places where we write into vectors with ghosts. #6841

bangerth wants to merge 3 commits into from
+22 −7

Conversation

@bangerth
Copy link
Contributor

@bangerth bangerth commented Jan 22, 2026

This fixes two places where we get tripped up by the new rules that we can't write into vectors with ghosts, see #6822. There are more places, which I'll get to tomorrow -- it's close to midnight already...

@bangerth bangerth mentioned this pull request Jan 22, 2026
Open
@bangerth
Copy link
Contributor Author

bangerth commented Jan 23, 2026

I pushed another commit that should fix several more places.

tjhei
tjhei reviewed Jan 23, 2026
View reviewed changes
source/simulator/helper_functions.cc Outdated

// Set the velocity initial guess to zero.
current_linearization_point.block(introspection.block_indices.velocities) = 0;
// Set the velocity initial guess to zero. Do this via a vector
Copy link
Member

@tjhei tjhei Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While correct, this shows to me that your change in deal.II might be too restrictive. Not only do you need to add a comment and many lines of code, this also is going to be slower than before.

Copy link
Contributor Author

@bangerth bangerth Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps. I think zeroing out a vector might be a special case I'd be willing to consider to allow on a vector with ghost element, though I'm not 100% sure I know how to support that for all vectors. I'm happy to discuss this over in deal.II land. It would be easy to undo these changes here if we want to change semantics, given that these commits contain nothing else.

@bangerth
Copy link
Contributor Author

bangerth commented Jan 23, 2026

The remaining failures are

	675 - nonlinear_channel_flow_velocities_Newton_Stokes (Failed)
	679 - nonlinear_channel_flow_velocities_Newton_Stokes_no_deviator (Failed)
	823 - periodic_box_mpi4 (Timeout)

The first two are in the Newton solver, which I would like to fix in a separate PR because it is entangled with @arnoldkk13 's #6804, which I need to think about. I don't know what's going on with periodic_box_mpi4, so would like to leave that to a separate PR as well.

In other words, my suggestion would be to consider this PR as complete and ready to merge if there is agreement about the direction.

gassmoeller
gassmoeller approved these changes Jan 23, 2026
View reviewed changes
Copy link
Member

@gassmoeller gassmoeller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to agree with Timo's point.

current_linearization_point.block(introspection.block_indices.velocities) = 0;

is almost never a shorthand for:

for (unsigned int i=0; i<size; ++i)
  current_linearization_point.block(introspection.block_indices.velocities)(i) = 0.0;

i.e. individually setting all entries to 0, which is not allowed for ghosted vectors. Instead it is semantically more similar to:

current_linearization_point.block(introspection.block_indices.velocities) = LinearAlgebra::Vector(introspection.index_sets.system_partitioning[introspection.block_indices.velocities],mpi_communicator);

i.e. reinitialize the whole vector to a new vector will all entries being zeroed out, which is already allowed for ghosted vectors. But I dont know about the different vector implementations and what it would entail to make this work.

For ASPECT I think we should just go ahead with these changes. Even if the changes in deal.II are modified and assignment of 0 is allowed the changes of this PR will still work, and the overhead cost should be small.

@tjhei
Copy link
Member

tjhei commented Jan 23, 2026

I would rather fix deal.II than putting a bandaid into ASPECT that we then need to remove again.

@bangerth bangerth mentioned this pull request Jan 24, 2026
Open
@bangerth
Copy link
Contributor Author

bangerth commented Jan 24, 2026

I opened an issue over with deal.II about this.

Of the three places this patch touches, only two create new vectors. For one, I pushed a commit that avoids the creation of the new vector -- we can just use swap() there. That's probably even more efficient than the code before. That just leaves the one place in simulator.cc. I could make that more efficient by asking whether the vector we're setting to zero is in fact already zero, which I suspect is the case most of the time. That would avoid creating the temp vector, though of course it still requires touching all vector elements once to determine whether the vector is zero. Would that be a compromise that would allow us to move forward so we can get the CI checks back working?

@tjhei
Copy link
Member

tjhei commented Jan 24, 2026

Would that be a compromise that would allow us to move forward so we can get the CI checks back working?

We can leave things as they are currently. Additional logic to check if the vector is already zero doesn't seem worth it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tjhei tjhei tjhei approved these changes

@gassmoeller gassmoeller gassmoeller approved these changes

Assignees

No one assigned

Labels

ready to test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bangerth @tjhei @gassmoeller